OCPEDGE-2448: Add/update TNF recovery tests to verify PacemakerHealthCheck - #31545

WaitForPacemakerEvent is hardcoded to openshift-etcd-operator via the pacemakerHealthCheckEventNamespace constant (line 227). This works for health-check controller events (PacemakerHealthy, PacemakerClusterInMaintenance, PacemakerNodeOffline, etc.), but the status collector emits PacemakerFencingEvent in the openshift-etcd namespace — its InvolvedObject is the pacemaker-status-collector CronJob which lives there (confirmed from CEO statuscollector.go which uses TargetNamespace = "openshift-etcd").

A follow-up story needs to assert fencing events after real fence operations (OCPSTRAT-3606 AC #2), and this helper cannot support that without a namespace parameter.

Suggestion: add a namespace parameter:

func WaitForPacemakerEvent(oc *exutil.CLI, namespace, reason string, since time.Time, timeout time.Duration) error {

Callers in this PR pass pacemakerHealthCheckEventNamespace explicitly — no behavior change. The follow-up can then pass "openshift-etcd" for fencing event assertions.

Also: the assertion strings in tnf_pacemaker_healthcheck.go lines 92 and 111 say "in openshift-etcd namespace" while this helper queries openshift-etcd-operator. Please fix those strings to match the actual namespace being queried.

The empty substring "" accepts any degraded message. At this point in the flow (after VM destruction + stonith confirm, before deleteNodeReferences), the CIB still lists both nodes — deleteNodeReferences deletes K8s resources (BMH, Machine, Node) but never runs pcs cluster node remove. The PR's own pcs cluster stop test comment confirms: "stopping corosync on a node does not remove it from the configured node count."

So NodeCountAsExpected stays True throughout, and the expected degraded reason is "is offline" (from the PacemakerNodeOffline path in CEO's healthcheck.go).

Two asks:

1. Tighten the substring to "is offline" — a regression where the controller fails to detect the offline node but degrades for another reason (e.g. stale CR) would pass silently with "".

2. After the degraded assertion, add a CR condition check for the destroyed node:

g.By("Verifying PacemakerCluster CR shows target node Online=False")
o.Eventually(func() error {
    pc, pcErr := apis.GetPacemakerCluster(oc)
    if pcErr != nil {
        return pcErr
    }
    return apis.ExpectNodeOnlineFalse(pc, testConfig.TargetNode.Name)
}, 2*time.Minute, utils.FiveSecondPollInterval).ShouldNot(o.HaveOccurred(),
    "Target node should show Online=False in PacemakerCluster CR after destruction")

This needs a new ExpectNodeOnlineFalse helper (same pattern as ExpectNodeFencingUnavailable). This is the only test where a node is physically destroyed, so it's the right place to verify the CR reflects that.

To reconcile with my other comment asking to collapse the Expect* functions: don't add a new ExpectNodeOnlineFalse — use the generic ExpectNodeCondition(pc, nodeName, etcdv1.NodeOnlineConditionType, metav1.ConditionFalse) from the collapse.

Also, use Eventually for the "is offline" substring check rather than requiring it on first observation — same stale-first race as the pcs-stop test (if the CronJob was pinned to the destroyed node, the first degraded message can be "is stale" before rotation delivers "is offline").

initially it was "is offline" but there is a race condition on the healthcheck controller that is causing some issues, hence we just log it first and then fail later.
Let me see how else we can do this.

SkipIfClusterIsNotHealthy checks nodes, COs, etcd pods, and voting members — but not PacemakerHealthCheck state. The dedicated PHC tests exist to verify accuracy of the health check pipeline, not just detect/clear cycles.

A collector bug that misreports FencingHealthy=False on a healthy cluster produces only a warning (CEO's getFencingWarnings — never trips Degraded), so neither the precondition gate nor ExpectPacemakerHealthCheckNotDegraded would catch it.

Two separate things needed:

1. Light check in SkipIfClusterIsNotHealthy (separate from this file): add Degraded=False + CR lastUpdated < 2min. This is a skip gate — if PHC is already broken before the test starts, skip rather than produce confusing failures.

2. Full ExpectPacemakerBaseline(oc) assertion at start and end of each test in this file and tnf_fencing_credentials.go. Check cluster conditions (Healthy=True, InService=True, NodeCountAsExpected=True) and per-node conditions (FencingAvailable=True, FencingHealthy=True, Member=True).

This is the test payload, not the gate. It's built on etcdv1 condition type constants, so it only breaks on an actual API break to the GA'd v1 type — not when CEO adds new conditions.

The informational approach is correct — blocking here would flake due to the collector CronJob pinning race (if pinned to the downed node, rotation takes 2-3 min, and a reboot can complete inside that window). Good design choice.

However, framework.Logf output disappears into raw logs. If degraded detection consistently fails across runs, nobody sees it.

The mechanism already exists: the preconditions package (see pkg/test/preconditions/preconditions.go) solves this exact problem with string markers. Tests log [precondition-check] and [precondition-skip], and the runner (detectPreconditionChecks in cmd_runsuite.go:1449) scans test.testOutputBytes across subprocess boundaries to produce a synthetic JUnit entry. No in-process state needed.

Ask: follow the same pattern:

1. Define markers (e.g. [phc-degraded-check] and [phc-degraded-miss]) — either extend the preconditions package or create a sibling.

2. In this informational path, log the markers:

framework.Logf("[phc-degraded-check] validating PHC degraded during graceful shutdown")
if err := apis.WaitForPacemakerHealthCheckDegraded(oc, "", pacemakerDegradedDetectionTimeout); err != nil {
    framework.Logf("[phc-degraded-miss] PHC degraded not observed during graceful shutdown: %v", err)
}

3. Add a detectPHCDegradedMisses scanner in the runner (alongside detectPreconditionChecks) producing a synthetic [sig-etcd] PacemakerHealthCheck degraded observed during disruptions JUnit entry. TRT gets pass-rate tracking; individual recovery tests stay non-flaky.

CEO's clearPacemakerDegradedCondition explicitly writes Degraded=False on the first healthy sync — it never leaves the condition absent after the controller starts running. An absent condition means the health check controller never ran at all, which is the one failure mode this wait should not mask.

If the controller is broken or not deployed, every test calling WaitForPacemakerHealthCheckCleared passes immediately on the first poll, silently.

Suggestion (preferred): require Status=False explicitly — absent fails at timeout with the diagnostics dump:

cond := findOperatorCondition(etcd, PacemakerHealthCheckDegradedCondition)
if cond == nil {
    lastErr = "condition not yet present on etcd operator"
    framework.Logf("WaitForPacemakerHealthCheckCleared: %s", lastErr)
    continue
}

Alternative: accept absent only after a bounded wait for the condition to appear (e.g. poll up to 2min for it to exist, then require False).

Blocking: This blocks for up to 15 minutes before validateEtcdRecoveryState(... false, true ...) on line 133. A graceful reboot completes well inside that window — by the time the wait finishes (or times out), the node is already back and promoted, so started=false, learner=true never matches and the test fails after 20+ minutes.

Same issue in the sequential/mixed cold-boot tests (lines 356, 407) where this wait sits between the first and second shutdown, changing what "sequential" means.

Fix: don't block the recovery path. Either start a background observer (go + atomic.Bool) that polls IsPacemakerHealthCheckDegraded every 5s, stop it after recovery, and evaluate at the end — or assert post-hoc via a PacemakerNodeOffline event with the since baseline.

Seven near-identical Expect* functions (lines 59–191) differ only in condition type and expected status. Collapse to:

func ExpectNodeCondition(pc *etcdv1.PacemakerCluster, nodeName, condType string, expected metav1.ConditionStatus) error
func ExpectClusterCondition(pc *etcdv1.PacemakerCluster, condType string, expected metav1.ConditionStatus) error

Keep the named wrappers as one-liners if you want caller readability, but the bodies shouldn't be duplicated.

This ~25-line pcs status text parser (duplicated again at line 360) extracts the stonith agent name by parsing bullet-prefixed lines with fields[1]. The PacemakerCluster CR already has Status.Nodes[].FencingAgents[].Name and .Method. Use GetPacemakerCluster and pick the agent whose parent node matches targetNode.Name — deterministic, no text parsing, and it's the object under test.

Raw DebugNodeRetryWithOptionsAndChroot(..., "bash", "-c", "sudo pcs ...") calls appear 12 times across this file and tnf_fencing_credentials.go, with ad-hoc 2>/dev/null; true in cleanup variants. services/pacemaker.go already has the *ViaDebug pattern. Add PcsPropertySetViaDebug, PcsClusterStart/StopViaDebug, PcsStonithEnable/DisableViaDebug, PcsStonithSetManagedViaDebug and use those.

nit: pacemakerDegradedDetectionTimeout (15m) is defined here but used by four files. healthCheckRecoveryTimeout (10m) is declared here but used by tnf_fencing_credentials.go. Export both from apis/pacemakerhealthcheck.go with the CEO derivation comment.

nit: Inconsistent race handling. The kubelet test got the re-induce loop (kubelet stop re-applied when Pacemaker recovers before snapshot). The etcd container-kill and process-crash tests hard-block on degraded with the same 1-minute snapshot race — the RA restarts etcd in well under a minute on process crash. Either apply the same re-induce pattern or use the background observer from the recovery tests.

nit: DeferCleanup restores state (disables maintenance / starts pcs cluster) but returns immediately. The next spec's SkipIfClusterIsNotHealthy (1-minute etcd precondition) can then skip instead of fail. Have the cleanup wait for WaitForPacemakerHealthCheckCleared (bounded) before returning.

nit: fmt.Fprintf(g.GinkgoWriter, ...) here and line 398 — the rest of the codebase uses framework.Logf for warnings.

nit: dumpHealthCheckDiagnostics only runs on wait timeout. Register it as a failure-time DeferCleanup in the PHC Describe (same pattern as deferDiagnosticsOnFailure) so any failing assertion gets the dump, not just timeouts.

nit: "10m minimum" is wrong. timeSinceLastValid in CEO's healthcheck.go:591 uses previous.CRLastUpdated, which is the frozen lastUpdated timestamp (only updated on non-Unknown syncs). The moment staleness fires at ~5m, timeSinceLastValid is already >5m → degrades in the same sync. The two thresholds are anchored to the same timestamp, not additive. The constants comment ("matches StatusStalenessThreshold") even says so.

Should read: "That is a 5m minimum" — and 15m is fine as a generous upper bound.